Extend existence check in URLImageDescriptor when running without OSGi #3106

ptziegler · 2025-07-11T15:17:26Z

This extends the check added in cff785d to also consider the case when OSGi isn't running. Otherwise a path to a non-existent file is returned, thus breaking the contract specified in the JavaDoc.

ptziegler · 2025-07-11T15:19:41Z

This e.g. breaks the scaled image data retrieval in DPIUtil:

Exception in thread "main" org.eclipse.swt.SWTException: i/o error (java.io.FileNotFoundException: C:\Eclipse\gmf-runtime-master\git\gmf-runtime\bundles\org.eclipse.gmf.runtime.gef.ui\bin\org\eclipse\gmf\runtime\gef\ui\internal\icons\[email protected] (The system cannot find the file specified))
	at org.eclipse.swt.SWT.error(SWT.java:4946)
	at org.eclipse.swt.SWT.error(SWT.java:4861)
	at org.eclipse.swt.graphics.ImageLoader.load(ImageLoader.java:199)
	at org.eclipse.swt.graphics.ImageDataLoader.load(ImageDataLoader.java:55)
	at org.eclipse.swt.graphics.Image$ImageFileNameProviderWrapper.loadImageData(Image.java:2293)
	at org.eclipse.swt.graphics.Image$BaseImageProviderWrapper.initializeHandleFromSource(Image.java:2251)
	at org.eclipse.swt.graphics.Image$BaseImageProviderWrapper.lambda$0(Image.java:2232)
	at java.base/java.util.HashMap.computeIfAbsent(HashMap.java:1220)
	at org.eclipse.swt.graphics.Image$BaseImageProviderWrapper.newImageData(Image.java:2237)
	at org.eclipse.swt.graphics.Image.getImageData(Image.java:1223)
	at org.eclipse.gmf.runtime.gef.ui.internal.l10n.Cursors.getScaledImageData(Cursors.java:117)
	at org.eclipse.gmf.runtime.gef.ui.internal.l10n.Cursors.lambda$0(Cursors.java:55)
	at org.eclipse.swt.internal.DPIUtil.lambda$1(DPIUtil.java:540)
	at org.eclipse.swt.internal.DPIUtil.getElementAtZoom(DPIUtil.java:569)
	at org.eclipse.swt.internal.DPIUtil.validateAndGetImageDataAtZoom(DPIUtil.java:540)
	at org.eclipse.swt.graphics.Cursor.win32_getHandle(Cursor.java:424)
	at org.eclipse.swt.widgets.Control.WM_SETCURSOR(Control.java:5483)
	at org.eclipse.swt.widgets.Shell.WM_SETCURSOR(Shell.java:2667)
	at org.eclipse.swt.widgets.Control.windowProc(Control.java:4856)
	at org.eclipse.swt.widgets.Canvas.windowProc(Canvas.java:336)
	at org.eclipse.swt.widgets.Decorations.windowProc(Decorations.java:1494)
	at org.eclipse.swt.widgets.Shell.windowProc(Shell.java:2378)
	at org.eclipse.swt.widgets.Display.windowProc(Display.java:5128)
	at org.eclipse.swt.internal.win32.OS.PeekMessage(Native Method)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3741)
	at test.Snippet.main(Snippet.java:28)
Caused by: java.io.FileNotFoundException: C:\Eclipse\gmf-runtime-master\git\gmf-runtime\bundles\org.eclipse.gmf.runtime.gef.ui\bin\org\eclipse\gmf\runtime\gef\ui\internal\icons\[email protected] (The system cannot find the file specified)
	at java.base/java.io.FileInputStream.open0(Native Method)
	at java.base/java.io.FileInputStream.open(FileInputStream.java:216)
	at java.base/java.io.FileInputStream.<init>(FileInputStream.java:157)
	at java.base/java.io.FileInputStream.<init>(FileInputStream.java:111)
	at org.eclipse.swt.graphics.ImageLoader.load(ImageLoader.java:196)
	... 23 more

github-actions · 2025-07-11T15:23:11Z

Test Results

2 778 files ±0 2 778 suites ±0 1h 38m 25s ⏱️ - 1m 17s
7 931 tests +1 7 702 ✅ ±0 228 💤 ±0 1 ❌ +1
23 348 runs +3 22 601 ✅ +2 746 💤 ±0 1 ❌ +1

For more details on these failures, see this check.

Results for commit d8c699b. ± Comparison against base commit 789b373.

♻️ This comment has been updated with latest results.

laeubi · 2025-07-14T16:40:27Z

bundles/org.eclipse.jface/src/org/eclipse/jface/resource/URLImageDescriptor.java

-				if (FILE_PROTOCOL.equalsIgnoreCase(url.getProtocol()))
-					return IPath.fromOSString(url.getFile()).toOSString();
+				if (FILE_PROTOCOL.equalsIgnoreCase(url.getProtocol())) {
+					String filePath = IPath.fromOSString(url.getFile()).toOSString();


can't we use Path.of(...) directly.

I think Path.of(url.toURI()).toString() is even less readable than IPath.fromOSString(url.getFile()).toOSString(), in addition to also having to deal with the URISyntaxException.

IPath.fromOSString(url.getFile()).toOSString()

to be honest this looks really bogus, why not simply

Suggested change

String filePath = IPath.fromOSString(url.getFile()).toOSString();

String filePath = url.getFile();

but I more was about that below you are using a path anyways so

p = Path.of(url.toURI()) if (Files.exists(Path.of(p))) { return p.toString(); }

why not simply

As mentioned, you now have to also worry about the potential URISyntaxException and are now depending on something ambiguous as a toString() method...

But as long as the method behaves consistently, I don't really mind either way.

What now raises the question if we should not extract it to an own method, and why we need an existence checks for files but not for regular URLs ... so maybe the caller will check for the existence anyways later on?

What now raises the question if we should not extract it to an own method, and why we need an existence checks for files but not for regular URLs

Because URLs are not used in combination with the ImageFileNameProvider. The reason while files get a special treatment is primarily performance and I don't think extracting this logic to a separate method only adds unnecessary complexity.

so maybe the caller will check for the existence anyways later on?

The caller will not. Otherwise this issue wouldn't even exist in the first place. And this is also not expected, as specified in the JavaDoc;

eclipse.platform.ui/bundles/org.eclipse.jface/src/org/eclipse/jface/resource/URLImageDescriptor.java

Line 247 in 789b373

* @return {@link String} or <code>null</code> if the file cannot be found

I don't think extracting this logic to a separate method only adds unnecessary complexity.

At least you would not need to adjust the logic in two places now ...

as specified in the JavaDoc;

If I read it correctly FileNameProvider it should only return null if the zoom != 100 .. but this seems flawed before.

At least you would not need to adjust the logic in two places now ...

Updated.

If I read it correctly FileNameProvider it should only return null if the zoom != 100 .. but this seems flawed before.

That is true and probably also why the IOException is explicitly logged in such a case. But that's something that goes beyond the scope of this PR.

@laeubi I've moved the conversion from URL to String to a separate method. Do you see anything else I've missed? Otherwise I'd merge this within the next days.

This extends the check added in cff785d to also consider the case when OSGi isn't running. Otherwise a path to a non-existent file is returned, thus breaking the contract specified in the JavaDoc.

laeubi

It looks good to me.

ptziegler · 2025-07-17T09:51:08Z

Test failure on MacOS is unrelated: #294

HeikoKlare · 2025-07-17T11:11:45Z

This change leads to a bunch of exceptions when starting a product for me. Seems to be related to image paths containing whitespaces.
It's caused by the change to use Path.of(...) as proposed here: #3106 (comment)
When I revert that one, it works as expected again.

java.net.URISyntaxException: Illegal character in path at index 81: file:/D:/dev/eclipse/platform-only/ws/.metadata/.plugins/org.eclipse.pde.core/SDK Test Product 2/org.eclipse.osgi/2342/0/.cp/icons/obj/iu_obj.svg
	at java.base/java.net.URI$Parser.fail(URI.java:2995)
	at java.base/java.net.URI$Parser.checkChars(URI.java:3166)
	at java.base/java.net.URI$Parser.parseHierarchical(URI.java:3248)
	at java.base/java.net.URI$Parser.parse(URI.java:3196)
	at java.base/java.net.URI.<init>(URI.java:645)
	at java.base/java.net.URL.toURI(URL.java:1220)
	at org.eclipse.jface.resource.URLImageDescriptor.getFilePath(URLImageDescriptor.java:272)
	at org.eclipse.jface.resource.URLImageDescriptor.getFilePath(URLImageDescriptor.java:256)
	at org.eclipse.jface.resource.URLImageDescriptor.lambda$0(URLImageDescriptor.java:65)

@ptziegler can you please have a look?

merks · 2025-07-17T11:32:43Z

I stopped following the long discussion here but URL.toURI() seems to always cause problems because we generally do a poor job encoding and this method expects a properly encoded URL...

ptziegler · 2025-07-17T11:56:25Z

I stopped following the long discussion here but URL.toURI() seems to always cause problems because we generally do a poor job encoding and this method expects a properly encoded URL...

Hence why I was reluctant to use it in the first place... I'm on it. The error is thrown when e.g. converting a file to URL via file.toURL(), but not when using e.g. file.toURI().toURL(). I'll play around for a bit but will probably go back to using the toOSString().

ptziegler · 2025-07-17T12:12:26Z

See #3115

ptziegler mentioned this pull request Jul 11, 2025

[90] Fix cursor initialization when using fractional scaling eclipse-gmf-runtime/gmf-runtime#93

Open

ptziegler force-pushed the url-image-path branch 2 times, most recently from 5c2261f to 33196c4 Compare July 14, 2025 16:16

laeubi reviewed Jul 14, 2025

View reviewed changes

ptziegler force-pushed the url-image-path branch 2 times, most recently from abd678c to 693d086 Compare July 15, 2025 05:32

Extend existence check in URLImageDescriptor when running without OSGi

d8c699b

This extends the check added in cff785d to also consider the case when OSGi isn't running. Otherwise a path to a non-existent file is returned, thus breaking the contract specified in the JavaDoc.

ptziegler force-pushed the url-image-path branch from 693d086 to d8c699b Compare July 15, 2025 05:33

laeubi approved these changes Jul 17, 2025

View reviewed changes

HeikoKlare mentioned this pull request Jul 17, 2025

SVG rasterization mac is low-resolution eclipse-platform/eclipse.platform.swt#2228

Open

ptziegler merged commit cee4631 into eclipse-platform:master Jul 17, 2025
16 of 18 checks passed

ptziegler deleted the url-image-path branch July 17, 2025 09:51

	String filePath = IPath.fromOSString(url.getFile()).toOSString();
	String filePath = url.getFile();

Extend existence check in URLImageDescriptor when running without OSGi #3106

Extend existence check in URLImageDescriptor when running without OSGi #3106

Uh oh!

Conversation

ptziegler commented Jul 11, 2025

Uh oh!

ptziegler commented Jul 11, 2025

Uh oh!

github-actions bot commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laeubi left a comment

Choose a reason for hiding this comment

Uh oh!

ptziegler commented Jul 17, 2025

Uh oh!

Uh oh!

HeikoKlare commented Jul 17, 2025

Uh oh!

merks commented Jul 17, 2025

Uh oh!

ptziegler commented Jul 17, 2025

Uh oh!

ptziegler commented Jul 17, 2025

Uh oh!

Uh oh!

github-actions bot commented Jul 11, 2025 •

edited

Loading